clear last applied configuration #385

mfrancisc · 2024-04-05T15:14:40Z

see https://redhat-internal.slack.com/archives/C06MJ2DVBU4/p1712310833089889

codecov · 2024-04-05T15:16:31Z

Codecov Report

Merging #385 (3004dc4) into master (32311a6) will increase coverage by 0.07%.
The diff coverage is 85.71%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #385      +/-   ##
==========================================
+ Coverage   77.66%   77.74%   +0.07%     
==========================================
  Files          46       46              
  Lines        1952     1959       +7     
==========================================
+ Hits         1516     1523       +7     
  Misses        376      376              
  Partials       60       60

Files	Coverage Δ
pkg/client/client.go	`88.57% <85.71%> (+0.60%)`	⬆️

alexeykazakov · 2024-04-05T16:13:15Z

pkg/client/client.go

-		newConfiguration = getNewConfiguration(obj)
+		// reset the previous config to avoid recursive embedding of the object
+		if _, found := obj.GetAnnotations()[LastAppliedConfigurationAnnotationKey]; found {
+			delete(obj.GetAnnotations(), LastAppliedConfigurationAnnotationKey)


I would rather move this to the getNewConfiguration() function. So the function always returns a clean object to be used in the last applied configuration annotation. And also would create a deep copy of the object in that getNewConfiguration() function before modifying it (removing the annotation). It won't break anything in the current code if we don't do it but it would make the function less error prone in case we modify the code in the future.

Good point, done in aacc2c6

I've also added the annotation to the existing obj in unit tests.

…stappliedconfig

mfrancisc · 2024-04-09T12:56:29Z

pkg/client/client.go

@@ -278,7 +287,7 @@ func ApplyUnstructuredObjectsWithNewLabels(ctx context.Context, cl client.Client
 	for _, unstructuredObj := range unstructuredObjects {
 		log.Info("applying object", "object_namespace", unstructuredObj.GetNamespace(), "object_name", unstructuredObj.GetObjectKind().GroupVersionKind().Kind+"/"+unstructuredObj.GetName())
 		MergeLabels(unstructuredObj, newLabels)
-		_, err := applyClient.ApplyObject(ctx, unstructuredObj)
+		_, err := applyClient.ApplyObject(ctx, unstructuredObj, SaveConfiguration(false))


Unfortunately for Unstructured objects it is still not working, so I'm keeping the saveConfiguration functionality disabled. I've tried multiple things:

removing the LastAppliedConfigurationAnnotationKey annotation from both new object before creating it

removing theLastAppliedConfigurationAnnotationKey annotation from the existing object before merging the annotations with the new object

removing the special handling of Unstructured objects in marshalObjectContent:

if newRes, ok := newResource.(runtime.Unstructured); ok { return json.Marshal(newRes.UnstructuredContent()) }

but nothing seems to work. The toolchain.dev.openshift.com/last-applied-configuration annotation is still being recursively embedded.

I'm also wondering if we really need this functionality at all. We could

a. trigger the Update of the object anyway , that should do anything if there are no changes on the object.
b. rely on kubectl.kubernetes.io/last-applied-configuration , how is this different from our custom annotation ?

Good points @mfrancisc. Not sure why we started using our annotation instead. Maybe @MatousJobanek remembers why.

it was an attempt to get closer to what kubectl does, to optimize the number of updates we do from our controllers and to have some control over what was updated and what not.
But I really doubt that it works as we hoped that it could work, so I'm for dropping the annotation 👍 to simplify the logic. But let's do it in a separate PR.

… present

MatousJobanek

Looks good, just some comments related to the tests

MatousJobanek · 2024-04-17T07:20:40Z

pkg/client/client.go

@@ -124,6 +124,7 @@ func (c ApplyClient) applyObject(ctx context.Context, obj client.Object, options
 	namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}
 	if err := c.Client.Get(ctx, namespacedName, existing); err != nil {
 		if apierrors.IsNotFound(err) {
+			obj.SetResourceVersion("") // reset resource version when creating to avoid error: resourceVersion should not be set on objects to be created


Do you know in which cases the provided object contained the resourceVersion?

It was happening with the new toolchaincluster_resource_controller. Unfortunately I don't have the logs anymore but creation of the objects was failing due to this ResourceVersion field being set. TBH I was confused about why this was happening, since the objects should be "new" from the templates directly.

didn't you see the failure as part of the unit tests? see the comment below - it could be the case.

I wasn't able to reproduce the failure in unit tests. you mean that we should pass a deepcopy of the objects here https://github.com/codeready-toolchain/toolchain-common/pull/385/files#diff-2895f30116602d8fe6b0545c186d4f81247fa4eaab049dab174c6c91c0036e08R291 ? Or something else ..

MatousJobanek · 2024-04-17T07:37:55Z

pkg/client/client_test.go

+
+	t.Run("updates of ServiceAccount", func(t *testing.T) {
+
+		t.Run("should update service account with last applied configuration", func(t *testing.T) {


since the "last-appplied-configuration" is used and is set to the same value, then there the SA shouldn't be updated.

good point! I've changed the name of the test in 91ec671. Thank you!

MatousJobanek · 2024-04-17T07:39:43Z

pkg/client/client_test.go

+			}
+			existingSA.SetAnnotations(existingLastAppliedAnnotation) // let's set the last applied annotation
+			cl, cli := newClient(t)
+			_, err := cl.ApplyRuntimeObject(context.TODO(), existingSA)


I noticed that we should pass a deep-copy of the object, otherwise, the generation as well as resourceVersion values would be set and would influence the late execution when the object that is passed to the function again:

Suggested change

_, err := cl.ApplyRuntimeObject(context.TODO(), existingSA)

_, err := cl.ApplyRuntimeObject(context.TODO(), existingSA.DeepCopyObject())

done in 91ec671

…stappliedconfig

sonarqubecloud · 2024-04-17T08:43:36Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
10.5% Duplication on New Code

See analysis details on SonarCloud

clear last applied configuration

c952c18

mfrancisc requested review from MatousJobanek, xcoulon, alexeykazakov, rajivnathan, ranakan19 and sbryzak as code owners April 5, 2024 15:14

alexeykazakov reviewed Apr 5, 2024

View reviewed changes

mfrancisc added 4 commits April 8, 2024 07:42

move cleanup

aacc2c6

Merge branch 'master' into fixlastappliedconfig

0b781ea

remove resourceversion and create removeAnnotation func

7ed09c1

Merge remote-tracking branch 'origin/fixlastappliedconfig' into fixla…

ae37c06

…stappliedconfig

mfrancisc commented Apr 9, 2024

View reviewed changes

mfrancisc requested a review from alexeykazakov April 9, 2024 15:44

This was referenced Apr 9, 2024

add toolchainclusterresources controller codeready-toolchain/host-operator#1007

Merged

add toolchainclusterresources controller codeready-toolchain/member-operator#555

Merged

mfrancisc and others added 2 commits April 15, 2024 22:04

fix apply object when saveconfiguration is disabled and annotation is…

cf8634b

… present

Merge branch 'master' into fixlastappliedconfig

d40b79d

MatousJobanek approved these changes Apr 17, 2024

View reviewed changes

mfrancisc added 2 commits April 17, 2024 10:39

PR comments

91ec671

Merge remote-tracking branch 'origin/fixlastappliedconfig' into fixla…

3004dc4

…stappliedconfig

mfrancisc merged commit d3c1484 into codeready-toolchain:master Apr 17, 2024
8 checks passed

mfrancisc deleted the fixlastappliedconfig branch April 17, 2024 08:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clear last applied configuration #385

clear last applied configuration #385

mfrancisc commented Apr 5, 2024

codecov bot commented Apr 5, 2024 •

edited

Loading

alexeykazakov Apr 5, 2024

mfrancisc Apr 8, 2024

mfrancisc Apr 9, 2024

mfrancisc Apr 9, 2024

alexeykazakov Apr 10, 2024

MatousJobanek Apr 17, 2024

MatousJobanek left a comment

MatousJobanek Apr 17, 2024

mfrancisc Apr 17, 2024

MatousJobanek Apr 17, 2024

mfrancisc Apr 17, 2024

MatousJobanek Apr 17, 2024

mfrancisc Apr 17, 2024

MatousJobanek Apr 17, 2024

mfrancisc Apr 17, 2024

sonarqubecloud bot commented Apr 17, 2024


		t.Run("updates of ServiceAccount", func(t *testing.T) {

		t.Run("should update service account with last applied configuration", func(t *testing.T) {

	_, err := cl.ApplyRuntimeObject(context.TODO(), existingSA)
	_, err := cl.ApplyRuntimeObject(context.TODO(), existingSA.DeepCopyObject())

clear last applied configuration #385

clear last applied configuration #385

Conversation

mfrancisc commented Apr 5, 2024

codecov bot commented Apr 5, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatousJobanek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 17, 2024

Quality Gate passed

codecov bot commented Apr 5, 2024 •

edited

Loading